Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add options.attachStacktrace #160

Merged
merged 25 commits into from
Nov 12, 2020
Merged

add options.attachStacktrace #160

merged 25 commits into from
Nov 12, 2020

Conversation

rxlabz
Copy link
Contributor

@rxlabz rxlabz commented Nov 10, 2020

📢 Type of change

  • Bugfix
  • New feature
  • Enhancement
  • Refactoring

📜 Description

  • add SentryOptions.attachStackTrace to attach stacktrace on captureMessage and captureEvent ( true by default )
  • filter 'package:sentry' frames

💡 Motivation and Context

https://develop.sentry.dev/sdk/unified-api/

💚 How did you test it?

new tests to verify if stacktrace is correctly sended

📝 Checklist

  • I reviewed submitted code
  • I added tests to verify changes
  • All tests passing
  • No breaking changes

🔮 Next steps

@rxlabz rxlabz marked this pull request as ready for review November 10, 2020 11:01
@marandaneto
Copy link
Contributor

missing this:

https://github.com/getsentry/sentry-dart/blob/main/dart/lib/src/sentry_exception_factory.dart#L30

this assignment should only be made if attachStacktrace is also enabled

- serialize event.stracktrace within a threads interface
@rxlabz
Copy link
Contributor Author

rxlabz commented Nov 10, 2020

missing this:

https://github.com/getsentry/sentry-dart/blob/main/dart/lib/src/sentry_exception_factory.dart#L30

this assignment should only be made if attachStacktrace is also enabled

Copy link
Contributor

@marandaneto marandaneto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@@ -73,6 +77,7 @@ class Hub {
try {
sentryId = await item.client.captureEvent(
event,
stackTrace: stackTrace,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about we make the stackTrace part of the hint?
The idea of hint was to have a single .. hint to deal with these extra things without having to expand the public API

Copy link
Contributor

@marandaneto marandaneto Nov 11, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the hint still could be used for other stuff as we do on Android, but stackTrace on dart is captured separately so it makes sense IMO to have this expanded API.
on Java, a stack trace is always part of the exception, which is not this case.
The other way would be adding a new field (that is also called stackTrace or sentryStackTrace) to the event, which is confusing.

@rxlabz rxlabz merged commit b2c229e into main Nov 12, 2020
@rxlabz rxlabz deleted the feat/attach-trace branch November 12, 2020 09:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants